Skip to content

[forward_port] Verify checksums command #31082

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
PVince81 merged 3 commits into from
Apr 11, 2018
Merged

[forward_port] Verify checksums command #31082

PVince81 merged 3 commits into from
Apr 11, 2018

Conversation

@IljaN
Copy link
Contributor

@IljaN IljaN commented Apr 11, 2018

Forward port of #31008

@IljaN IljaN requested review from DeepDiver1975 and PVince81 April 11, 2018 10:47
@IljaN IljaN changed the title Verify checksums command forward port [forward_port] Verify checksums command Apr 11, 2018
@IljaN IljaN mentioned this pull request Apr 11, 2018
Closed
@IljaN IljaN self-assigned this Apr 11, 2018
@codecov
Copy link

codecov bot commented Apr 11, 2018 •
edited
Loading

Codecov Report

Merging #31082 into master will increase coverage by 0.03%.
The diff coverage is 85.05%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #31082      +/-   ##
============================================
+ Coverage     62.47%    62.5%   +0.03%     
- Complexity    18237    18268      +31     
============================================
  Files          1145     1146       +1     
  Lines         68313    68397      +84     
  Branches       1234     1234              
============================================
+ Hits          42678    42753      +75     
- Misses        25274    25283       +9     
  Partials        361      361
Flag Coverage Δ Complexity Δ
#javascript 52.03% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 63.7% <85.05%> (+0.03%) 18268 <26> (+31) ⬆️
Impacted Files Coverage Δ Complexity Δ
lib/private/Files/Storage/Wrapper/Checksum.php 100% <100%> (ø) 23 <0> (ø) ⬇️
lib/private/Files/Cache/Scanner.php 86.47% <100%> (+0.19%) 114 <0> (+3) ⬆️
lib/private/Files/ObjectStore/NoopScanner.php 61.53% <50%> (-5.13%) 5 <0> (+2)
apps/files/lib/Command/VerifyChecksums.php 85.71% <85.71%> (ø) 26 <26> (?)
lib/private/Files/Node/Node.php 86.76% <0%> (+1.47%) 62% <0%> (ø) ⬇️
lib/private/Files/Node/File.php 95.65% <0%> (+2.89%) 32% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e44172...2846d98. Read the comment docs.

PVince81
PVince81 approved these changes Apr 11, 2018
View reviewed changes
Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@PVince81 PVince81 merged commit 2897421 into master Apr 11, 2018
@PVince81 PVince81 deleted the verify_checksums_command_forward_port branch April 11, 2018 12:52
@mrow4a mrow4a mentioned this pull request May 6, 2018
Merged
1 task
mrow4a
mrow4a reviewed May 6, 2018
View reviewed changes
tests/lib/Files/ViewTest.php
return rtrim($checksumString);
}

public function testFilePutContentsClearsChecksum() {
Copy link
Contributor

@mrow4a mrow4a May 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @IljaN @PVince81 you removed that unit test, but did you cover that logic with another unit test? Where do you unit test condition to clear the checksum?

Copy link
Contributor Author

@IljaN IljaN May 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The checksum must not be cleared anymore, the checksum wrapper recalculates the new checksum on file_put_contents. The above tests is from the old checksum impl. where the client just sent some checksum and it got written to the db without any checks.

Copy link
Contributor Author

@IljaN IljaN May 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests are here: https://github.com/owncloud/core/edit/mutex_for_checksum_stream/tests/lib/Files/Storage/Wrapper/ChecksumTest.php?pr=/owncloud/core/pull/31102

Checksums are also covered by acceptance tests:

core/tests/acceptance/features/apiMain/checksums.feature

Line 2 in 7bc9eae

Feature: checksums

@lock
Copy link

lock bot commented Jul 31, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jul 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@mrow4a mrow4a mrow4a left review comments

@DeepDiver1975 DeepDiver1975 Awaiting requested review from DeepDiver1975

+1 more reviewer

@PVince81 PVince81 PVince81 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@IljaN IljaN

Labels

3 - To Review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@IljaN @PVince81 @mrow4a @DeepDiver1975